Automated review summary

Requested changes for one P2 performance regression: the new blanket multi-child gate fixes the unsafe mixed key/value COUNT DISTINCT case, but it also disables valid partial/parallel scanning for all-key multi-column COUNT DISTINCT. The inline comment contains the concrete plan, impact, and fix/test boundary.

Critical checkpoint conclusions:

• Goal and correctness: count(distinct k6, v7) on an aggregate-key table now correctly gets PREAGGREGATION OFF, preventing partial SUM states from changing the DISTINCT tuples. The implementation is broader than required and regresses the safe count(distinct k5, k6) plan.
• Scope and focus: the two-file patch is mechanically small and readable, but children().size() > 1 is not the smallest semantic predicate; supported multi-child COUNT DISTINCT arguments need per-scan validation.
• Parallel paths and performance: both Nereids SET_PREAGG_STATUS registrations use this rule. For AGG_KEYS/UNIQUE-MOR scans, the unnecessary OFF decision requires a full storage merge and can remove an otherwise eligible scan from the BE parallel-scan path. DUP_KEYS and UNIQUE-MOW/MOR-as-DUP bypasses are unchanged.
• Testing: the added EXPLAIN is a valid before-fail/after-pass trigger for the mixed key/value bug, but notContains ... PREAGGREGATION: ON does not prove the intended OFF reason and there is no positive all-key boundary. No result file is involved.
• Compatibility and FE/BE propagation: no function symbol, storage format, configuration, or thrift field changes are introduced; the existing preaggregation flag is forwarded unchanged.
• Concurrency, lifecycle, persistence, transactions, and writes: none are introduced or modified by this patch.
• Error handling and observability: the new branch fails closed and EXPLAIN exposes the OFF reason; no additional log or metric is needed.
• Other paths: aliases, nested aggregates, joins/multiple scans, nullable/duplicate keys, expression shapes, and later DISTINCT splitting were checked. No additional independent issue was substantiated.

User focus: no additional review focus was provided.

Validation: static review only. The review environment explicitly prohibits builds and test execution, so the added regression case was inspected but not run.

Review completion: every changed file and candidate was swept, deduplicated, and resolved; normal full-review and risk-focused agents converged within the three-round cap.

Automated review summary

Requested changes for three P1 correctness regressions in aggregate-key preaggregation classification. The direct count(DISTINCT k6, v7) case is now OFF and direct all-key count(DISTINCT k6, k5) is ON, but adjacent legal plans can still receive ON and observe partial storage rows.

Critical checkpoint conclusions:

• Goal and correctness: not fully achieved; three wrong-result paths are described inline.
• Scope and minimality: the patch is small and readable, but the input-slot-set criterion is broader than the supported argument semantics and assumes every slot was classified.
• Parallel paths and performance: both SetPreAggStatus registrations are affected. The safe deterministic all-key optimization is preserved; the unsafe ON decisions change correctness rather than only performance. Planning cost is negligible.
• Tests and results: the added EXPLAIN checks cover the direct mixed/all-key boundaries, but not an IF/CASE first argument with a later value column, a volatile key expression, or a multi-project derived alias. The weak OFF-reason assertion is already covered by the existing thread. No local build or test was run because the review contract prohibits it; current style, license, and basic checks are green, with no targeted regression result at this head.
• Compatibility, persistence, transactions, writes, FE/BE protocol, concurrency, lifecycle, configuration, and memory: no independent issue applies.
• Error handling and observability: the existing EXPLAIN OFF reason is sufficient; no new log or metric is needed.
• Existing review context: the direct all-key discussion was deduplicated.
• User focus: none was specified.

Review completion: every changed file and candidate was swept; three full/risk-focused rounds converged with NO_NEW_VALUABLE_FINDINGS on this final ledger and comment set.

Validation: static review only; builds and tests were prohibited by the governing review prompt.

Request changes

The all-key multi-argument DISTINCT path and the current-head fixes for the existing threads are directionally correct, but this review found four additional issues: three correctness/planning failures and one planner-complexity regression. Because M-004 was discovered in the third and final allowed round, the mandated convergence protocol ended without every reviewer returning NO_NEW_VALUABLE_FINDINGS; this review is therefore incomplete under the automation contract, although every candidate discovered so far has a verified disposition and is included below.

Critical checkpoint conclusions:

• Goal and data correctness: the direct all-key multi-column DISTINCT case is enabled and the mixed-value cases are rejected, but volatile expressions can still observe partial-row multiplicity (M-002), and mixed validation can enable one scan using another scan's SUM value (M-003).
• Scope and clarity: the patch remains within preaggregation classification and its regression suite, but eager alias composition introduces both redundant map copying (M-001) and an uncaught expression-limit failure (M-004).
• Concurrency and lifecycle: no concurrent shared state, lock ordering, asynchronous lifecycle, or static-initialization concern is introduced; the context is per rewrite.
• Configuration and compatibility: no new configuration, FE/BE protocol, storage-format, rolling-upgrade, or persisted-state change is present. M-004 incorrectly bypasses the existing expression width/depth limits rather than introducing a new setting.
• Parallel paths and conditions: value-only, key-only, mixed IF/CASE, multi-argument DISTINCT, sibling projects, and join paths were traced. The filter/join/grouping and other-table volatile paths are not covered by the new local guard, and mixed key/value ownership is not scan-relative.
• Tests and results: the added EXPLAIN cases cover direct all-key/mixed DISTINCT, local volatile aggregate arguments/conditions, and the repaired sibling alias case. Coverage is still missing for volatile filter/join/grouping and other-table aggregates, cross-scan foreign-SUM multiplicity, and over-limit nested project composition. This runner's review contract prohibited builds/tests; validation was static only, and thirdparty/installed/bin/protoc is absent.
• Performance: M-001 adds O(N^2) prefix copying across projected branches; no other material hot-path regression was substantiated.
• Observability: existing PREAGGREGATION ON/OFF reasons are sufficient for this rule; no new metric or logging requirement was identified.
• Transactions, persistence, data writes, and FE/BE variable propagation: not applicable to these planner-only changes.
• User focus: no additional user-provided focus was supplied.